-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Time in Txs Response #4007
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4007 +/- ##
===========================================
- Coverage 59.76% 59.75% -0.02%
===========================================
Files 211 211
Lines 15055 15058 +3
===========================================
Hits 8998 8998
- Misses 5435 5438 +3
Partials 622 622 |
} | ||
|
||
// NewResponseResultTx returns a TxResponse given a ResultTx from tendermint | ||
func NewResponseResultTx(res *ctypes.ResultTx, tx Tx) TxResponse { | ||
func NewResponseResultTx(res *ctypes.ResultTx, tx Tx, timestamp string) TxResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a test case for this? It would give a tiny boost to our coverage report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test case for the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, it sounds dumb but it'd be easy and we would reduce the impact on the coverage report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvement and nice cleanup of that package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor comments, otherwise LGTM
res, err := node.TxSearch(query, prove, page, perPage) | ||
if err != nil { | ||
return nil, err | ||
func queryTxs(cliCtx context.CLIContext, cdc *codec.Codec, tag string, delegatorAddr string) ([]sdk.TxResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I think we can delete the staking query tx endpoint. It's a dup from regular tx query, the main difference is that you don't need to specify tags, which is silly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^ would be api-breaking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but let's save that for another issue/PR -- would like to avoid as much scope creep as possible. Mind opening an issue?
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
TxResponse
when querying for tx(s)query.go
utils.go
closes: #3238
/cc @faboweb @dogemos
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry:
sdkch add [section] [stanza] [message]
rereviewed
Files changed
in the github PR explorerFor Admin Use: